Skip to content

Conversation

@recalci
Copy link
Contributor

@recalci recalci commented Nov 7, 2024

Add TCPCI functions and add driver for Richtek RT1715.

@recalci recalci force-pushed the usbc_tcpc_rt1715 branch 4 times, most recently from 84315d9 to 4cd7a1c Compare November 10, 2024 22:29
@recalci recalci marked this pull request as draft December 5, 2024 18:17
@recalci recalci marked this pull request as ready for review December 10, 2024 15:21
@recalci recalci force-pushed the usbc_tcpc_rt1715 branch 2 times, most recently from 8bacd7b to 5d3b6b4 Compare December 10, 2024 15:49
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you enabling both SOP' and SOP'' messages here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that both Numaker and STM32 enable them as well. However, I couldn't find the datasheet for the PS8xxx series, so I suspect that it may not support SOP'' messages.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PS8xxx does support SOP' and SOP''. However, the API documentation for tcpc_sop_prime_enable() indicates this command is used to enable SOP' messages, with no mention of SOP''.

The ChromeOS code that USB-C support is based on does enable SOP' and SOP'' for this command.

Do you mind updating the documentation for tcpc_sop_prime_enable() to note that this enables both SOP' and SOP''?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Please review.

@recalci recalci force-pushed the usbc_tcpc_rt1715 branch 2 times, most recently from 55c7eb8 to a8bb603 Compare December 14, 2024 12:54
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PS8xxx does support SOP' and SOP''. However, the API documentation for tcpc_sop_prime_enable() indicates this command is used to enable SOP' messages, with no mention of SOP''.

The ChromeOS code that USB-C support is based on does enable SOP' and SOP'' for this command.

Do you mind updating the documentation for tcpc_sop_prime_enable() to note that this enables both SOP' and SOP''?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't correct. You're writing the TCPC register number into a pointer.

Suggested change
buf[0].buf = TCPC_REG_RX_BUFFER;
uint8_t tcpc_reg = TCPC_REG_RX_BUFFER;
buf[0].buf = *tcpc_reg;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my fault. fixed

Copy link
Contributor

@keith-zephyr keith-zephyr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. One last ask. Please add a build test that links in this driver.

See #83187 for an example.

Locally, you can run ./scripts/twister -ivc -s drivers.usb.build.i2c to verify the test.

@zephyrbot zephyrbot added the area: USB Universal Serial Bus label Dec 20, 2024
@recalci
Copy link
Contributor Author

recalci commented Dec 20, 2024

Looking good. One last ask. Please add a build test that links in this driver.

See #83187 for an example.

Locally, you can run ./scripts/twister -ivc -s drivers.usb.build.i2c to verify the test.

Thanks for your review.
The commit has been added. In order to ensure that everything works well together, this PR contains #83187 now. If #83187 is merged first, I will rebase and update this PR.

@keith-zephyr
Copy link
Contributor

Looking good. One last ask. Please add a build test that links in this driver.
See #83187 for an example.
Locally, you can run ./scripts/twister -ivc -s drivers.usb.build.i2c to verify the test.

Thanks for your review. The commit has been added. In order to ensure that everything works well together, this PR contains #83187 now. If #83187 is merged first, I will rebase and update this PR.

#83187 has now merged.

@recalci
Copy link
Contributor Author

recalci commented Dec 21, 2024

Looking good. One last ask. Please add a build test that links in this driver.
See #83187 for an example.
Locally, you can run ./scripts/twister -ivc -s drivers.usb.build.i2c to verify the test.

Thanks for your review. The commit has been added. In order to ensure that everything works well together, this PR contains #83187 now. If #83187 is merged first, I will rebase and update this PR.

#83187 has now merged.

Just updated. Local tests passed.

keith-zephyr
keith-zephyr previously approved these changes Dec 26, 2024
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking comment. This change isn't related to using the common tcpci functions.

Add generic functions that will be common to all TCPCI compliant drivers.

Signed-off-by: Jianxiong Gu <[email protected]>
Add support for RT1715.

Signed-off-by: Jianxiong Gu <[email protected]>
This commit modifies the ps8xxx.c driver to use the generic TCPCI function.

Signed-off-by: Jianxiong Gu <[email protected]>
Clearly mention that this function enables both SOP' and SOP'' messages.

Signed-off-by: Jianxiong Gu <[email protected]>
Add a build test to verify richtek,rt1715 driver builds correctly.

Signed-off-by: Jianxiong Gu <[email protected]>
@kartben kartben merged commit eefbed2 into zephyrproject-rtos:main Jan 15, 2025
24 checks passed
@recalci recalci deleted the usbc_tcpc_rt1715 branch January 29, 2025 11:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: USB Universal Serial Bus area: USB-C

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants